Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make ClassMetadata generic #2308

Merged
merged 2 commits into from
May 21, 2021
Merged

Make ClassMetadata generic #2308

merged 2 commits into from
May 21, 2021

Conversation

franmomu
Copy link
Contributor

Q A
Type improvement
BC Break no
Fixed issues

Summary

Following the changes from doctrine/persistence, this PR adds generics to ClassMetadata and to Iterator.

Copy link
Member

@alcaeus alcaeus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I appreciate all the work you do here and in our other projects to document these generic classes! 💚

What do you think @franmomu, should we do another check for other generics in ODM before 2.3, or should we defer that to after our release?

Comment on lines +16 to +20
# This cannot be solved the way it is, see https://github.com/vimeo/psalm/issues/5788
-
message: "#^Return type \\(Doctrine\\\\ODM\\\\MongoDB\\\\Mapping\\\\ClassMetadataFactory\\) of method Doctrine\\\\ODM\\\\MongoDB\\\\DocumentManager\\:\\:getMetadataFactory\\(\\) should be compatible with return type \\(Doctrine\\\\Persistence\\\\Mapping\\\\ClassMetadataFactory\\<Doctrine\\\\Persistence\\\\Mapping\\\\ClassMetadata\\<object\\>\\>\\) of method Doctrine\\\\Persistence\\\\ObjectManager\\:\\:getMetadataFactory\\(\\)$#"
count: 1
path: lib/Doctrine/ODM/MongoDB/DocumentManager.php
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not even going to pretend I understand half of what's going on here, but just to confirm I got this somewhat right: the problem is that we can't correctly say "this manager returns a class metadata factory which creates ORM metadata instances", is that correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, exactly, but in this case if I try to make ClassMetadataFactory generic, I get an error from phpstan I guess because of this generic template of generic maybe.

@alcaeus alcaeus added this to the 2.3.0 milestone May 21, 2021
@alcaeus alcaeus self-assigned this May 21, 2021
@alcaeus alcaeus mentioned this pull request May 21, 2021
@franmomu
Copy link
Contributor Author

What do you think @franmomu, should we do another check for other generics in ODM before 2.3, or should we defer that to after our release?

From the top of the head maybe https://github.com/doctrine/mongodb-odm/blob/2.2.x/lib/Doctrine/ODM/MongoDB/Aggregation/Builder.php and all iterators, but that could be some work, so maybe it's better to defer it after the release.

Let me make my last PR in this branch for upgrading doctrine/coding-standard.

@alcaeus alcaeus merged commit ddc12b2 into doctrine:2.3.x May 21, 2021
@alcaeus
Copy link
Member

alcaeus commented May 21, 2021

Thanks @franmomu!

@franmomu franmomu deleted the more_generics branch May 21, 2021 08:05
@alcaeus alcaeus modified the milestones: 2.3.0-alpha1, 2.3.0 Sep 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants